-
Notifications
You must be signed in to change notification settings - Fork 831
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(k6): add support for k8s loading/unloading of Seldon CRs #5563
Conversation
This adds initial support for k8s via the `USE_KUBE_CONTROL_PLANE` environment variable: - models/pipelines/experiments in components/model.js are modified to also return CR yamls - updates functions in components/utils.js (including `setupBase(...)` and `teardownBase(...)` to use xk6-kubernetes, when configured - all scenarios already using `setupBase(...)` should work unchanged **Which issue(s) this PR fixes:** - INFRA-949 (internal issue) Extend existing k6 scenarios to use xk6-kubernetes - [ ] Test functionality in kind - [ ] Extend existing k6 scenarios to use xk6-kubernetes
- works: model creation + load - fails: getting model status via k8s (permissions) - fails: deleting/tear down models (permissions)
- name: SCHEDULER_ENDPOINT | ||
value: "${SCHEDULER_ENDPOINT}:9004" | ||
- name: INFER_HTTP_ITERATIONS | ||
value: "1" | ||
- name: INFER_GRPC_ITERATIONS | ||
value: "1" | ||
- name: MODELNAME_PREFIX | ||
value: "tfsimplea,pytorch_cifar10a,tfmnista,mlflow_winea,irisa" | ||
value: "tfsimplea,pytorch-cifar10a,tfmnista,mlflow-winea,irisa" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
names changed because the ones with _ were not getting loaded in k8s
@@ -97,32 +101,32 @@ export function getModelInferencePayload(modelName, inferBatchSize) { | |||
const shape = [inferBatchSize, 16] | |||
var httpBytes = [] | |||
var grpcBytes = [] | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes from here to line 208 are linting changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lovely! this is a great feature moving forward.
I guess the scenarios we care about can use USE_KUBE_CONTROL_PLANE
, i.e. infer_constant_rate.js
and infer_constant_vu.js
.
@@ -0,0 +1,103 @@ | |||
import { Kubernetes } from "k6/x/kubernetes"; | |||
import { getConfig } from '../components/settings.js' | |||
import { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the future we might want also to check the status via kube. not a blocker for this PR though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, was already planning for that. The xk6-kubernetes
extension is a bit wierd in that it doesn't have built-in functionality for waiting on a given condition/status. Also, it throws exceptions for any unexpected conditions, so I'll have to code that a bit defensively (via repeated resource get
s).
tests/k6/components/k8s.js
Outdated
} | ||
|
||
export function loadModel(modelName, data, awaitReady=true) { | ||
//console.log(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, will clean those up. They were there in the scheduler code so I've left them for dev debugging
|
||
export function loadModel(modelName, data, awaitReady=true) { | ||
//console.log(data) | ||
if(!seldonObjExists(seldonObjectType.MODEL, modelName, namespace)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have to check if the model already exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a model already exists here we get an exception. The exception likely happens only when the loaded model has the same CR as an existing one (it is an apply after all).
tests/k6/components/k8s.js
Outdated
} | ||
|
||
export function unloadModel(modelName, awaitReady=true) { | ||
// console.log("Unloading model "+modelName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove?
tests/k6/components/k8s.js
Outdated
} | ||
|
||
export function loadPipeline(pipelineName, data, awaitReady=true) { | ||
//console.log(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove?
tests/k6/components/model.js
Outdated
@@ -1,3 +1,7 @@ | |||
// import { dump as yamlDump } from "../import/js-yaml.mjs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove?
for (var i = 0; i < 16 * inferBatchSize; i++) { | ||
grpcBytes.push("MQ=="); // base64 of 1 | ||
httpBytes.push("97") | ||
} | ||
const payload = { | ||
"http": {"inputs":[{"name":"INPUT0","data":httpBytes,"datatype":"BYTES","shape":shape},{"name":"INPUT1","data":httpBytes,"datatype":"BYTES","shape":shape}]}, | ||
"grpc": {"inputs":[{"name":"INPUT0","contents":{"bytes_contents":grpcBytes},"datatype":"BYTES","shape":shape},{"name":"INPUT1","contents":{"bytes_contents":grpcBytes},"datatype":"BYTES","shape":shape}]} | ||
"http": { "inputs": [{ "name": "INPUT0", "data": httpBytes, "datatype": "BYTES", "shape": shape }, { "name": "INPUT1", "data": httpBytes, "datatype": "BYTES", "shape": shape }] }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: how did you reformat this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was the automated formatter from intelliJ (we should try to find a cli formatter and add linting/formatting to the makefile)
tests/k6/components/model.js
Outdated
"storageUri": uri, | ||
"requirements": modelTemplate.requirements, | ||
"memory": (memoryBytes == null) ? modelTemplate.memoryBytes : memoryBytes, | ||
"minReplicas": 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if we need minReplicas
, it will trigger I think autoscaling of models which we might want to do.
maybe at least align with the grpc api path.
@@ -0,0 +1,5 @@ | |||
export const seldonObjectType = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
- cleanup commented debug calls - remove minReplicas from CRs
This adds initial support for k8s via the
USE_KUBE_CONTROL_PLANE
environment variable:setupBase(...)
andteardownBase(...)
to use xk6-kubernetes, when configuredsetupBase(...)
should work unchangedWhich issue(s) this PR fixes:
TODO